Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(ImmutableDevices): allow passing onRowClick function #2080

Closed
wants to merge 3 commits into from

Conversation

mkholjuraev
Copy link
Contributor

Enables consumer apps to pass their own onRowClick function. This will enable to have different behavior of system name column depending on the consumer.

@mkholjuraev mkholjuraev requested a review from a team as a code owner November 2, 2023 09:20
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
...rc/components/ImmutableDevices/ImmutableDevices.js 100.00% <100.00%> (ø)

📢 Thoughts on this report? Let us know!

@bastilian
Copy link
Member

Wouldn't the onRowClick of the TitleColumn[0] (vie the entity) already allow to do what this is meant to allow?

[0] https://github.com/RedHatInsights/insights-inventory-frontend/blob/master/src/components/InventoryTable/TitleColumn.js#L12

@mkholjuraev
Copy link
Contributor Author

mkholjuraev commented Nov 7, 2023

Hmm, that would require tweaking TitleColumn as it does not allow redirect with activeTab query param. There is an onRowClick prop in InventoryTable readily available that maps to TitleColumn onRowClick function that has the same usage. Do we want to tweak TitleColumn?

@bastilian
Copy link
Member

Do we want to tweak TitleColumn?

We always want to enhance what we have instead of introducing something new. We have had several occasions in the past where we simply wanted to change where the link navigates to. It seems to me it is time to enhance the onRowClick of the TitleColum.

@mkholjuraev
Copy link
Contributor Author

mkholjuraev commented Nov 8, 2023

It seems to me it is time to enhance the onRowClick of the TitleColum.

I am happy to improve. I am thinking about what would be the best way to go as TitleColumn is used in almost all places where InventoryTable is consumed. It is provided to default columns. Would I need to pass a new onTitleColumnClick function from top-level components to down the TitleColumn component?

@mkholjuraev
Copy link
Contributor Author

@bastilian I investigated the scope of changes needed to improve the TitleColumn. It feels to me that it has large-scale changes and testing are needed. I would not want to be blocked with this while there are more important tasks waiting for edge-parity work. I can create a task for this improvement.

@bastilian
Copy link
Member

@mkholjuraev What changes would be "large-scale"? It would likely just need the addition of a onRowClick property to the entities provided to the InventoryTable.

@mkholjuraev
Copy link
Contributor Author

I am not able to find a clean way to pass onRowClick to TitleColumn. Can you clarify your idea? By large-scale, I mean passing this onRowClick would require me to make changes in multiple functions and test the change in multiple applications to make sure it does not affect them.

@bastilian
Copy link
Member

@mkholjuraev Is there no getEntities or fetch function that can add a onRowClick prop to each entity it returns?

@computercamplove
Copy link
Contributor

/retest

@mkholjuraev mkholjuraev marked this pull request as draft November 10, 2023 13:09
@mkholjuraev
Copy link
Contributor Author

Putting in draft mode for #2087

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants